fix: compute AgentToken domain separator dynamically for replay protection#5140
Closed
KHHH2312 wants to merge 2 commits into
Closed
fix: compute AgentToken domain separator dynamically for replay protection#5140KHHH2312 wants to merge 2 commits into
KHHH2312 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds chain-ID-aware EIP-712 domain separator recomputation to AgentToken to protect against replay attacks following a chain fork, along with tests and Solidity compiler config updates.
Changes:
- Replace immutable
DOMAIN_SEPARATORstorage with a view function that recomputes the separator whenblock.chainiddiffers from the deployment chain id. - Update
permitto call the newDOMAIN_SEPARATOR()function. - Add tests for dynamic domain separator and permit; bump Solidity to 0.8.24 with
cancun/viaIRsettings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| contracts/token/AgentToken.sol | Refactors domain separator into a chain-ID-aware view function; also adds an extraneous header comment block. |
| test/AgentTokenDomainSeparator.test.js | Adds tests for dynamic domain separator and permit flow. |
| hardhat.config.js | Switches to compilers array, bumps to 0.8.24, enables viaIR and cancun EVM. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+68
to
+69
| function DOMAIN_SEPARATOR() public view returns (bytes32) { | ||
| return block.chainid == _initialChainId ? _initialDomainSeparator : _computeDomainSeparator(_hashedName, _hashedVersion); |
Comment on lines
+43
to
+44
| it("should successfully execute a permit using the dynamic domain separator", async function () { | ||
| const [owner, spender] = await ethers.getSigners(); |
Comment on lines
+11
to
+14
| beforeEach(async function () { | ||
| [owner] = await ethers.getSigners(); | ||
|
|
||
| const AgentToken = await ethers.getContractFactory("AgentToken"); |
Comment on lines
+8
to
+9
| const initialChainId = 31337; // Hardhat default | ||
| const newChainId = 12345; |
Comment on lines
+7
to
+9
| version: "0.8.24", | ||
| settings: { | ||
| evmVersion: "cancun", |
- Added eip712Domain() function to comply with EIP-5267. - Cleaned up shadowed variables and unused code in tests.
Author
|
I have pushed an update addressing Copilot's review feedback:
|
|
Unfortunately the changes in this PR didn't fully resolve the issue. Please rework your solution and submit a new pull request within 2 hours. Make sure to review the acceptance criteria in the linked issue and verify all conditions are met before resubmitting. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
/claim #162
💳 Payment: USDC | 0x43991A9dC8Ddf492eab6E55685644c2cb9B001D2 | Base
Changes